New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Presto's date_add UDF with TimestampAndTimeZone and DST #11353

Closed

kevinwilfong wants to merge 1 commit into facebookincubator:main from kevinwilfong:export-D64982873

Contributor

kevinwilfong commented Oct 25, 2024

Summary:
Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT. It needs to check if
the time zone has changed between the original timestamp and the timestamp after
the addition and apply the change in time zone offset to the mills since epoch to
account for this long or short day.

Note that the for units less than a day, this does not apply. E.g. if you add 24 hours
to a TimestampWithTimeZone and it cross a DST boundary, we do not need to apply
the change in offset like you would if you had added 1 day.

Differential Revision: D64982873

facebook-github-bot added the CLA Signed label

Contributor

facebook-github-bot commented Oct 25, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873

facebook-github-bot added the fb-exported label

netlify bot commented Oct 25, 2024 •

edited

Loading

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`3ddb9e0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6722677506837e00083b477c

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

97ec37f

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to check if
the time zone has changed between the original timestamp and the timestamp after
the addition and apply the change in time zone offset to the mills since epoch to
account for this long or short day.

Note that the for units less than a day, this does not apply.  E.g. if you add 24 hours
to a TimestampWithTimeZone and it cross a DST boundary, we do not need to apply
the change in offset like you would if you had added 1 day.

Differential Revision: D64982873

kevinwilfong force-pushed the export-D64982873 branch from 8bea75a to 97ec37f Compare

October 29, 2024 17:25

Contributor

facebook-github-bot commented Oct 29, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

cacb024

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873

kevinwilfong force-pushed the export-D64982873 branch from 97ec37f to cacb024 Compare

October 29, 2024 17:33

Contributor

facebook-github-bot commented Oct 29, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

28d84a6

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873

kevinwilfong force-pushed the export-D64982873 branch from cacb024 to 28d84a6 Compare

October 29, 2024 17:54

Contributor

facebook-github-bot commented Oct 29, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_diff UDF with TimestampAndTimeZone and DST

4fab016

Summary:
Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
facebookincubator#11353

Differential Revision: D65165953

kevinwilfong force-pushed the export-D64982873 branch from 28d84a6 to cd84f85 Compare

October 29, 2024 20:36

kevinwilfong mentioned this pull request

Fix Presto's date_diff UDF with TimestampAndTimeZone and DST #11380

Closed

Contributor

facebook-github-bot commented Oct 29, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

f5cad86

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873

kevinwilfong force-pushed the export-D64982873 branch from cd84f85 to f5cad86 Compare

October 29, 2024 21:12

Contributor

facebook-github-bot commented Oct 29, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

73446f5

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_diff UDF with TimestampAndTimeZone and DST (faceboo…

…kincubator#11380)

Summary:

Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
facebookincubator#11353

Differential Revision: D65165953

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

b4e8b4a

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Reviewed By: xiaoxmeng

Differential Revision: D64982873

kevinwilfong force-pushed the export-D64982873 branch from f5cad86 to b4e8b4a Compare

October 30, 2024 17:04

Contributor

facebook-github-bot commented Oct 30, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

3ddb9e0

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Reviewed By: xiaoxmeng

Differential Revision: D64982873

kevinwilfong force-pushed the export-D64982873 branch from b4e8b4a to 3ddb9e0 Compare

October 30, 2024 17:05

Contributor

facebook-github-bot commented Oct 30, 2024

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_add UDF with TimestampAndTimeZone and DST (facebook…

c255b78

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Reviewed By: xiaoxmeng

Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request


          Fix Presto's date_diff UDF with TimestampAndTimeZone and DST (faceboo…

dad4d83

…kincubator#11380)

Summary:

Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
facebookincubator#11353

Reviewed By: xiaoxmeng

Differential Revision: D65165953

xiaoxmeng approved these changes

View reviewed changes

facebook-github-bot closed this in

1c0c533

facebook-github-bot pushed a commit that referenced this pull request


          Fix Presto's date_diff UDF with TimestampAndTimeZone and DST (#11380)

7377bc8

Summary:
Pull Request resolved: #11380

Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
#11353

Reviewed By: xiaoxmeng

Differential Revision: D65165953

fbshipit-source-id: 7817ffd31c9e078c3078e861fe3813135bf0f2b8

facebook-github-bot added the Merged label

Contributor

facebook-github-bot commented Oct 30, 2024

This pull request has been merged in 1c0c533.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed fb-exported Merged